Add block cache trait #1192

Oscar-Pepper · 2024-02-20T19:20:09Z

Adds a BlockCache trait for BlockSource which aims to generalise the block source for consumers who are not implementing an SQLite database. This lays some of the groundwork for #1184 by removing cyclic dependencies and generalising the sync engine for wallets with different implementations

Closes #1170

zcash_client_backend/src/data_api/chain.rs

rust-toolchain.toml

…` with methods for managing a block cache.

…nning.rs

Oscar-Pepper · 2024-03-07T10:03:33Z

#1217 shows how this BlockCache trait is implemented into the #1184 sync engine

zancas · 2024-03-08T16:15:12Z

This looks ready to go! What's the protocol for approving a workflow?

codecov · 2024-03-08T16:46:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.76%. Comparing base (e795174) to head (7301c0c).
Report is 58 commits behind head on main.

❗ Current head 7301c0c differs from pull request most recent head 48e3ff1. Consider uploading reports for the commit 48e3ff1 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1192      +/-   ##
==========================================
+ Coverage   63.59%   63.76%   +0.16%     
==========================================
  Files         121      121              
  Lines       13945    14009      +64     
==========================================
+ Hits         8869     8933      +64     
  Misses       5076     5076

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Cargo.toml

zcash_client_backend/Cargo.toml

… tokio deps and moved tokio rt-multi-thread feature to dev-dependencies

daira

utACK modulo documentation for short reads, if they are allowed.

- Added read error documentation - Changed name of `cache_tip` method to `get_tip_height` - Improved doc comments

zancas · 2024-03-13T16:21:54Z

Oh hi! What needs to be done to approve this workflow?

daira

utACK

zancas · 2024-03-14T17:09:55Z

I think this is ready to land. I think @Oscar-Pepper has already addressed @str4d 's requested changes.

str4d

Reviewed c62b5b2

zcash_client_backend/CHANGELOG.md

zcash_client_backend/src/data_api/chain.rs

zcash_client_backend/Cargo.toml

…nd replace with async trait

zancas · 2024-03-21T20:14:58Z

It looks like this workflow is ready to be kicked...

Oscar-Pepper · 2024-03-25T16:21:36Z

Hi @str4d I've made the requested changes and it's ready for review again

str4d

Reviewed as of 7301c0c.

zcash_client_backend/src/data_api/chain.rs

str4d · 2024-03-29T19:01:02Z

zcash_client_backend/src/data_api/chain.rs

+
+    /// Inserts a vec of compact blocks into the block cache.
+    ///
+    /// Returns `Ok(())` on success, otherwise returns an error.


Document whether this should return an error if the blocks aren't contiguous. It probably doesn't matter here if they aren't, but we should make sure devs are clear on what the expected behaviour is here.

I did not want to constrain an insert to a contiguous range of blocks and i will make that clear in the doc comment. I dont fully recall off the top of my head but there was a case where I realised, either from your current implemenation or a potential future optimisation, where it may be beneficial to be able to insert non-sequential ranges in one call.

zcash_client_backend/src/data_api/chain.rs

Oscar-Pepper · 2024-03-30T12:04:23Z

Reviewed as of 7301c0c.

Hi @str4d, thanks for the review, I have replied to and made all requested changes except for adding &mut which we should discuss if its felt this is neccessary as its not clear to me how to implement 1169-sync-engine this way or further concurrent optimisations.

I have sent for review again.

str4d

utACK. There are some further changes I want to make (like placing this behind a feature flag), but I can do that when I rebase #1184 on top of this.

Oscar-Pepper force-pushed the add_block_cache_trait branch 3 times, most recently from ba0b06b to 7441a69 Compare February 23, 2024 14:38

zancas reviewed Feb 24, 2024

View reviewed changes

zcash_client_backend/src/data_api/chain.rs Show resolved Hide resolved

Oscar-Pepper force-pushed the add_block_cache_trait branch from 94d5f5e to e7003ee Compare February 28, 2024 17:23

nuttycom reviewed Feb 29, 2024

View reviewed changes

zcash_client_backend/src/data_api/chain.rs Outdated Show resolved Hide resolved

nuttycom reviewed Feb 29, 2024

View reviewed changes

rust-toolchain.toml Outdated Show resolved Hide resolved

Oscar-Pepper mentioned this pull request Mar 1, 2024

zcash_client_backend: Implement async wallet synchronization function #1184

Merged

Oscar-Pepper force-pushed the add_block_cache_trait branch 3 times, most recently from e4b7044 to 523b152 Compare March 4, 2024 16:38

zcash_client_backend: Added BlockCache trait extending `BlockSource…

1b4e5fa

…` with methods for managing a block cache.

Oscar-Pepper force-pushed the add_block_cache_trait branch from 523b152 to 1b4e5fa Compare March 4, 2024 16:56

Oscar-Pepper marked this pull request as ready for review March 4, 2024 18:34

Oscar-Pepper requested review from nuttycom and fluidvanadium March 4, 2024 18:34

Oscar-Pepper added 2 commits March 5, 2024 18:45

zcash_client_backend: Updated CHANGELOG.md

3852ba0

zcash_client_backend: Resolve merge conflicts in CHANGELOG.md and sca…

b888b86

…nning.rs

str4d requested changes Mar 8, 2024

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

zcash_client_backend/Cargo.toml Outdated Show resolved Hide resolved

zcash_client_backend: Added a sync feature to BlockCache for optional…

773af08

… tokio deps and moved tokio rt-multi-thread feature to dev-dependencies

daira previously approved these changes Mar 11, 2024

View reviewed changes

Oscar-Pepper dismissed daira’s stale review via 3802f4a March 12, 2024 13:55

requested changes:

ea991b7

- Added read error documentation - Changed name of `cache_tip` method to `get_tip_height` - Improved doc comments

Oscar-Pepper force-pushed the add_block_cache_trait branch from 97611f1 to ea991b7 Compare March 12, 2024 15:03

zcash_client_backend: Resolved merge conflicts in CHANGELOG.md

e89a1ac

Oscar-Pepper requested review from zancas and daira March 12, 2024 15:07

Oscar-Pepper requested a review from str4d March 12, 2024 15:07

Oscar-Pepper added a commit to Oscar-Pepper/librustzcash that referenced this pull request Mar 13, 2024

Fixed build errors rebasing on changes from zcash#1192

77d27a0

zcash_client_backend: Resolve merge conflicts in CHANGELOG.md

c62b5b2

daira previously approved these changes Mar 14, 2024

View reviewed changes

str4d requested changes Mar 15, 2024

View reviewed changes

zcash_client_backend/CHANGELOG.md Outdated Show resolved Hide resolved

zcash_client_backend/CHANGELOG.md Outdated Show resolved Hide resolved

zcash_client_backend/src/data_api/chain.rs Outdated Show resolved Hide resolved

zcash_client_backend/Cargo.toml Outdated Show resolved Hide resolved

Requested changes: remove tokio specific code from BlockCache trait a…

9c09567

…nd replace with async trait

Oscar-Pepper dismissed daira’s stale review via 9c09567 March 20, 2024 14:11

zcash_client_backend: Resolved merge conflicts in CHANGELOG.md

212decf

Oscar-Pepper requested review from str4d and daira March 20, 2024 14:18

zcash_client_backend: Removed async from insert method

7301c0c

str4d requested changes Mar 29, 2024

View reviewed changes

Made requested changes to str4d's review of 7301c0c

48e3ff1

Oscar-Pepper requested a review from str4d March 30, 2024 12:01

str4d approved these changes Apr 1, 2024

View reviewed changes

str4d merged commit 25b8404 into zcash:main Apr 1, 2024

Oscar-Pepper mentioned this pull request Aug 31, 2024

1169 sync engine with block cache trait #1217

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add block cache trait #1192

Add block cache trait #1192

Oscar-Pepper commented Feb 20, 2024 •

edited

Loading

Oscar-Pepper commented Mar 7, 2024

zancas commented Mar 8, 2024

codecov bot commented Mar 8, 2024 •

edited

Loading

daira left a comment

zancas commented Mar 13, 2024

daira left a comment

zancas commented Mar 14, 2024

str4d left a comment

zancas commented Mar 21, 2024

Oscar-Pepper commented Mar 25, 2024

str4d left a comment

str4d Mar 29, 2024

Oscar-Pepper Mar 30, 2024

Oscar-Pepper commented Mar 30, 2024

str4d left a comment

Add block cache trait #1192

Add block cache trait #1192

Conversation

Oscar-Pepper commented Feb 20, 2024 • edited Loading

Oscar-Pepper commented Mar 7, 2024

zancas commented Mar 8, 2024

codecov bot commented Mar 8, 2024 • edited Loading

Codecov Report

daira left a comment

Choose a reason for hiding this comment

zancas commented Mar 13, 2024

daira left a comment

Choose a reason for hiding this comment

zancas commented Mar 14, 2024

str4d left a comment

Choose a reason for hiding this comment

zancas commented Mar 21, 2024

Oscar-Pepper commented Mar 25, 2024

str4d left a comment

Choose a reason for hiding this comment

str4d Mar 29, 2024

Choose a reason for hiding this comment

Oscar-Pepper Mar 30, 2024

Choose a reason for hiding this comment

Oscar-Pepper commented Mar 30, 2024

str4d left a comment

Choose a reason for hiding this comment

Oscar-Pepper commented Feb 20, 2024 •

edited

Loading

codecov bot commented Mar 8, 2024 •

edited

Loading